Add support for configuring PoE for cisco equipment #2635
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2635 +/- ##
==========================================
+ Coverage 55.43% 55.46% +0.03%
==========================================
Files 567 567
Lines 41120 41175 +55
==========================================
+ Hits 22793 22838 +45
- Misses 18327 18337 +10 ☔ View full report in Codecov by Sentry. |
stveit
force-pushed
the
poe-cisco
branch
2 times, most recently
from
c3fff35
to
361dfa5
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
Kudos, SonarCloud Quality Gate passed!
|
python/nav/portadmin/snmp/cisco.py
Outdated
| :returns: A dict mapping interfaces to their discovered PoE state. | ||
| The key matches the `ifindex` attribute for the related | ||
| Interface object. | ||
| The value will be None if the interface does not support PoE. | ||
| """ |
There was a problem hiding this comment.
I approved the base updates to the ManagementHandler API, but now I'm wondering if using ifindexes in response values was such a good idea. The only other ManagementHandler methods that return a dict mapping for interfaces uses the interface names as the key, not their SNMP ifindex, which may be meaningless if the protocol handler isn't talking SNMP (like the Juniper handler, which speaks NETCONF-ish)
There was a problem hiding this comment.
So Interface.ifname should be used instead?
There was a problem hiding this comment.
I also have some apprehensions for how the interfaces argument is handled.
Is this part really necessary:
If this (interfaces) parameter is omitted,
the default behavior is to filter on all Interface objects
registered for this device.
If the caller of the function wants to get poe state for all interfaces, then they can just put that in the interfaces argument directly. This extra functionality adds needless complexity imo
Basically I am saying that maybe it shouldn't be optional at all
There was a problem hiding this comment.
If the caller of the function wants to get poe state for all interfaces, then they can just put that in the
interfacesargument directly. This extra functionality adds needless complexity imo
Removing it will just move the complexity to the caller instead.
However, we should keep the design as it is in this PR, to remain consistent, and let's have this discussion in a new issue instead.
stveit
force-pushed
the
poe-cisco
branch
2 times, most recently
from
3e1c44c
to
774399f
Compare
mocking is unavoidable here, but can atleast mock as far away as possible (the manager itself)
these dont have to be accessed in any way in the test, so its cleaner to activate them through the decorator than as an argument
|
Kudos, SonarCloud Quality Gate passed!
|
Fixes #2632.
Adds backend for getting current PoE state for interfaces and changing the state.